[Consensus] Use SignerIndices in CollectionGuarantee (3/4) #2140

zhangchiqing · 2022-03-11T00:29:56Z

https://github.com/dapperlabs/flow-go/issues/6181

This PR:

Replace SignerIDs with SignerIndices in CollectionGuarantee definition
Added ReferenceBlockID to CollectionGuarantee in order to look up collection by cluster
Fixing all test cases for signer indices related changes. (I passed all the tests with base branch as master. After tests are passing, I switched base branch to leo/signer-indices so that only new changes are retained)

github-actions · 2022-03-15T23:41:08Z

FVM Benchstat comparison

This branch with compared with the base branch onflow:feature/signer-indices commit 0bd50cf

The command (for i in {1..N}; do go test ./fvm --bench . --tags relic -shuffle=on; done) was used.

Bench tests were run a total of 7 times on each branch.

Results

	old.txt	new.txt
	time/op		delta
RuntimeNFTBatchTransfer-2	130ms ± 3%	128ms ± 1%	~	(p=0.053 n=7+7)
RuntimeTransaction/reference_tx-2	31.4ms ± 2%	32.0ms ± 4%	~	(p=0.259 n=7+7)
RuntimeTransaction/convert_int_to_string-2	33.2ms ± 4%	33.2ms ± 3%	~	(p=0.902 n=7+7)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-2	34.9ms ± 2%	34.8ms ± 4%	~	(p=0.731 n=7+6)
RuntimeTransaction/get_signer_address-2	32.2ms ± 1%	32.3ms ± 3%	~	(p=0.945 n=6+7)
RuntimeTransaction/get_public_account-2	35.6ms ± 2%	35.4ms ± 3%	~	(p=0.318 n=7+7)
RuntimeTransaction/get_account_and_get_balance-2	540ms ± 3%	537ms ± 2%	~	(p=0.902 n=7+7)
RuntimeTransaction/get_account_and_get_available_balance-2	441ms ± 4%	439ms ± 4%	~	(p=0.456 n=7+7)
RuntimeTransaction/get_account_and_get_storage_used-2	42.2ms ± 2%	42.3ms ± 4%	~	(p=1.000 n=6+7)
RuntimeTransaction/get_account_and_get_storage_capacity-2	398ms ± 2%	401ms ± 2%	~	(p=0.295 n=6+7)
RuntimeTransaction/get_signer_vault-2	40.5ms ± 1%	40.4ms ± 3%	~	(p=0.445 n=6+7)
RuntimeTransaction/get_signer_receiver-2	50.5ms ± 2%	50.7ms ± 1%	~	(p=0.234 n=7+6)
RuntimeTransaction/transfer_tokens-2	187ms ± 2%	186ms ± 2%	~	(p=0.456 n=7+7)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-2	38.1ms ± 1%	38.2ms ± 2%	~	(p=0.902 n=7+7)
RuntimeTransaction/load_and_save_long_string_on_signers_address-2	39.6ms ± 1%	40.0ms ± 4%	~	(p=0.535 n=7+7)
RuntimeTransaction/create_new_account-2	1.18s ± 3%	1.17s ± 3%	~	(p=0.805 n=7+7)
RuntimeTransaction/call_empty_contract_function-2	37.0ms ± 2%	36.9ms ± 2%	~	(p=0.902 n=7+7)
RuntimeTransaction/emit_event-2	53.0ms ± 7%	51.5ms ± 5%	~	(p=0.053 n=7+7)

zhangchiqing · 2022-03-17T04:47:19Z

cmd/bootstrap/cmd/clusters.go

@@ -35,6 +36,11 @@ func constructClusterAssignment(partnerNodes, internalNodes []model.NodeInfo, se
 		assignments[i%len(assignments)] = append(assignments[i%len(assignments)], node.NodeID)
 	}

+	// in place sort to order the assignment in canonical order


This was to address this issue

In order to ensure the identities are sorted in canonical order, I decided to sort it in the source, so that the clusters will have the sorted identities list, and no need to sort on every call to ClusterByChainID

⚠️ We will also need to apply this sort when incorporating the service event here

durkmurder · 2022-03-18T11:34:19Z

consensus/hotstuff/committees/leader/leader_selection_test.go

@@ -301,6 +301,7 @@ func TestZeroStakedNodeWillNotBeSelected(t *testing.T) {
 	})

 	t.Run("fuzzy set", func(t *testing.T) {
+		t.Skip()


Why it is skipped?

I disabled it initially because it runs very slowly, and I didn't know why.

Now I just realized master has changed the number of iteration to 1, so I'm going to bring the change here.

Now it's passing. Thanks for catching

durkmurder · 2022-03-18T12:31:04Z

engine/execution/ingestion/engine_test.go

@@ -423,6 +447,7 @@ func TestExecuteOneBlock(t *testing.T) {
 }

 func Test_OnlyHeadOfTheQueueIsExecuted(t *testing.T) {
+	unittest.SkipUnless(t, unittest.TEST_FLAKY, "flaky test")


was it flaky or became?

durkmurder · 2022-03-18T16:29:49Z

engine/execution/ingestion/engine_test.go

@@ -106,6 +108,7 @@ func runWithEngine(t *testing.T, f func(testingContext)) {
 	var engine *Engine

 	defer func() {
+		log.Info().Msgf("clean up tests")


Do we still need it? Looks like it was used for testing

This is still here - can we remove it?

durkmurder · 2022-03-18T16:39:22Z

utils/unittest/fixtures.go

@@ -846,7 +855,7 @@ func SignerIndicesFixture(n int) []byte {
 	for i := 0; i < n; i++ {
 		list[i] = i
 	}
-	indices, _ := packer.EncodeSignerIndices(list, 10)
+	indices, _ := packer.EncodeSignerIndices(list, 1)


Suggested change

indices, _ := packer.EncodeSignerIndices(list, 1)

indices, _ := packer.EncodeSignerIndices(list, n)

jordanschalm · 2022-03-21T15:03:30Z

consensus/hotstuff/committees/leader/leader_selection_test.go

@@ -304,7 +304,7 @@ func TestZeroStakedNodeWillNotBeSelected(t *testing.T) {
 		rng, err := random.NewChacha20PRG(someSeed, []byte("leader_selec"))
 		require.NoError(t, err)

-		for i := 0; i < 100; i++ {
+		for i := 0; i < 1; i++ {


Should we just remove the for loop?

jordanschalm · 2022-03-21T15:09:13Z

engine/access/rest/util/select_filter_test.go

+	// enable to update test case if there is change in the models.Block struct
+	// _ = ioutil.WriteFile("example_select_filter.json", marshalled, 0644)
+
+	byteValue, err := ioutil.ReadFile("example_select_filter.json")
+	require.NoError(t, err)
+
+	require.Equal(t, string(byteValue), string(marshalled))


👍 Good idea, thanks for updating

jordanschalm · 2022-03-21T15:09:47Z

engine/consensus/ingestion/core.go

@@ -68,7 +70,7 @@ func (e *Core) OnGuarantee(originID flow.Identifier, guarantee *flow.CollectionG
 	log := e.log.With().
 		Hex("origin_id", originID[:]).
 		Hex("collection_id", guaranteeID[:]).
-		Int("signers", len(guarantee.SignerIDs)).
+		// Int("signers", len(guarantee.SignerIDs)).


should we remove this?

I think we can just print signer indices, right?

This is still commented - can we log signer indices instead?

jordanschalm · 2022-03-21T15:16:02Z

engine/consensus/ingestion/core.go

+
+	// cluster not found by the chain ID
+	if errors.Is(err, protocol.ErrClusterNotFound) {
+		return engine.NewInvalidInputErrorf("cluster not found by chain ID %v, %v", guarantee.ChainID, err)


Suggested change

return engine.NewInvalidInputErrorf("cluster not found by chain ID %v, %v", guarantee.ChainID, err)

return engine.NewInvalidInputErrorf("cluster not found by chain ID %v, %w", guarantee.ChainID, err)

Using the generic format verb won't wrap the error.

jordanschalm · 2022-03-21T15:18:45Z

engine/consensus/ingestion/core.go

+	// determine whether signers reach minimally required stake threshold
+	threshold := hotstuff.ComputeStakeThresholdForBuildingQC(clusterMembers.TotalStake()) // compute required stake threshold


Suggested change

// determine whether signers reach minimally required stake threshold

threshold := hotstuff.ComputeStakeThresholdForBuildingQC(clusterMembers.TotalStake()) // compute required stake threshold

// determine whether signers reach minimally required weight threshold

threshold := hotstuff.ComputeStakeThresholdForBuildingQC(clusterMembers.TotalStake()) // compute required weight threshold

I will need to update stake and weight when I merge master, will do a clean up at that time.

jordanschalm · 2022-03-21T15:59:32Z

utils/unittest/fixtures.go

@@ -1561,6 +1570,16 @@ func QuorumCertificatesWithSignerIDsFixtures(n uint, opts ...func(*flow.QuorumCe
 	return qcs
 }

+func QuorumCertificatesWithAssignments(assignment flow.AssignmentList) []*flow.QuorumCertificateWithSignerIDs {


Suggested change

func QuorumCertificatesWithAssignments(assignment flow.AssignmentList) []*flow.QuorumCertificateWithSignerIDs {

func QuorumCertificatesFromAssignments(assignment flow.AssignmentList) []*flow.QuorumCertificateWithSignerIDs {

Match WithClusterQCsFromAssignments

jordanschalm · 2022-03-21T16:02:20Z

module/packer/signers.go

+	"github.com/onflow/flow-go/model/flow"
+)
+
+func FilterByIndices(identities flow.IdentityList, indices []int) (flow.IdentityList, error) {


Could you add a godoc here?

jordanschalm · 2022-03-21T16:04:17Z

module/finalizer/collection/finalizer.go

-					Signature: step.ParentVoterSigData,
+					ChainID:          header.ChainID,
+					SignerIndices:    step.ParentVoterIndices,
+					Signature:        step.ParentVoterSigData, // TODO: to remove because it's not easily verifiable by consensus nodes


Suggested change

Signature: step.ParentVoterSigData, // TODO: to remove because it's not easily verifiable by consensus nodes

Signature: nil, // TODO: to remove because it's not easily verifiable by consensus nodes

The signature isn't being validated so I think it is better to set it to nil now.

jordanschalm · 2022-03-21T16:08:49Z

cmd/bootstrap/cmd/clusters.go

@@ -35,6 +36,11 @@ func constructClusterAssignment(partnerNodes, internalNodes []model.NodeInfo, se
 		assignments[i%len(assignments)] = append(assignments[i%len(assignments)], node.NodeID)
 	}

+	// in place sort to order the assignment in canonical order


⚠️ We will also need to apply this sort when incorporating the service event here

jordanschalm · 2022-03-21T16:09:59Z

model/flow/cluster.go

+// The caller must ensure each assignment contains identities ordered in canonical order, so that
+// each cluster in the returned cluster list is ordered in canonical order as well.
 func NewClusterList(assignments AssignmentList, collectors IdentityList) (ClusterList, error) {


Can we check for canonical ordering here and return an error if not?

This comment is still outstanding. I think it's better if the constructor returns an error when it is asked to create an invalid cluster list.

Addressed here

durkmurder · 2022-03-22T12:47:35Z

model/flow/cluster.go

@@ -68,6 +70,7 @@ func NewClusterList(assignments AssignmentList, collectors IdentityList) (Cluste
 			cluster = append(cluster, participant)
 			delete(lookup, participantID)
 		}
+


Suggested change

durkmurder · 2022-03-22T12:52:38Z

engine/consensus/ingestion/core.go

+	// TODO: validate checksum
+	guarantorIndices, err := packer.DecodeSignerIndices(guarantee.SignerIndices, len(clusterMembers))
+	if err != nil {
+		return engine.NewInvalidInputErrorf("could not decode guarantor indices: %v", err)


Suggested change

return engine.NewInvalidInputErrorf("could not decode guarantor indices: %v", err)

return engine.NewInvalidInputErrorf("could not decode guarantor indices: %w", err)

jordanschalm · 2022-03-25T21:42:07Z

engine/execution/ingestion/engine_test.go

@@ -106,6 +108,7 @@ func runWithEngine(t *testing.T, f func(testingContext)) {
 	var engine *Engine

 	defer func() {
+		log.Info().Msgf("clean up tests")


This is still here - can we remove it?

jordanschalm · 2022-03-25T21:42:39Z

engine/consensus/ingestion/core.go

@@ -68,7 +70,7 @@ func (e *Core) OnGuarantee(originID flow.Identifier, guarantee *flow.CollectionG
 	log := e.log.With().
 		Hex("origin_id", originID[:]).
 		Hex("collection_id", guaranteeID[:]).
-		Int("signers", len(guarantee.SignerIDs)).
+		// Int("signers", len(guarantee.SignerIDs)).


This is still commented - can we log signer indices instead?

jordanschalm · 2022-03-25T21:45:42Z

model/flow/account.go

@@ -3,10 +3,10 @@
 package flow

 import (
-	"encoding/json"
-
 	"github.com/pkg/errors"


Hm, this is still using the old errors package from before it was included in the standard library.

We can remove this import and use fmt.Errorf instead

jordanschalm · 2022-03-25T21:55:32Z

model/flow/assignment/sort.go

+		assignment := identities[:]
+		flow.IdentifierList(assignment).Sort(order.IdentifierCanonical)


Suggested change

assignment := identities[:]

flow.IdentifierList(assignment).Sort(order.IdentifierCanonical)

assignment := flow.IdentifierList(identities[:]).Sort(order.IdentifierCanonical)

⚠️ Sort copies the input, then sorts and returns the copy. I don't think the sort currently would be applied to assignment, we need to use the return value of Sort.

Good catch. Thanks

jordanschalm · 2022-03-25T21:56:53Z

model/flow/cluster.go

+// The caller must ensure each assignment contains identities ordered in canonical order, so that
+// each cluster in the returned cluster list is ordered in canonical order as well.
 func NewClusterList(assignments AssignmentList, collectors IdentityList) (ClusterList, error) {


This comment is still outstanding. I think it's better if the constructor returns an error when it is asked to create an invalid cluster list.

canonical order

jordanschalm · 2022-03-28T19:27:22Z

model/flow/assignment/sort_test.go

+)
+
+// Check that FromIdentifierLists will sort the identifierList in canonical order
+func TestSort(t *testing.T) {


jordanschalm · 2022-03-28T19:33:00Z

model/flow/cluster.go

 			participant, found := lookup[participantID]
 			if !found {
 				return nil, fmt.Errorf("could not find collector identity (%x)", participantID)
 			}
 			cluster = append(cluster, participant)
 			delete(lookup, participantID)
+
+			if i > 0 {
+				if bytes.Compare(prev[:], participantID[:]) > 0 {


Suggested change

if bytes.Compare(prev[:], participantID[:]) > 0 {

if !IdentifierCanonical(prev, participantID) {

Directly use the canonical ordering function, otherwise this might break if the canonical ordering changed.

Another suggestion:

Now that we have canonical ordering defined for both []Identity and []Identifier, we could make some changes to keep them consistent:

define the Identity ordering in terms of the Identifier ordering

// model/flow/order/identity.go func Canonical(identity1 *flow.Identity, identity2 *flow.Identity) bool { return IdentifierCanonical(identity1.NodeID, identity2.NodeID) }

add a test case to make sure the canonical orderings match

identities := unittest.IdentityListFixture() assert.Equal(t, identities.Sort(Canonical).NodeIDs(), identities.NodeIDs().Sort(IdentifierCanonical))

* wip * wip * starting to fix tests * adding tests * happy path test * Added toDo for fixing tests for unhappy paths * • fixed packer tests • consolidated logic for checking the padded bits * re-gen mocks * fix validPadding * fix findguarantors * refactor ingestion/core.go * move FindGuarantors to protocol * remove commented code * fix name * fix tests * small refactor * fix import * Apply suggestions from code review Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> * fix error type * fix type * fix error message * update comments * update comment * update tests * fix identity_test * fix tests * fix unittest * fix cluster tests * fix tests * fixtures ingestion engine tests * fix execution_test * fix consensus inclusion tests * fix bootstrap constraint check * fix cycle dep from NewClusterList (#2225) Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

* implement IdentitiesByIndices * convert signer index * implement encoding and decoding * add test cases * remove signer indices file * fix Packer * add comments * remove IdentitiesByIndices * update mock * fix test cases * add comments * remove fields * fix ParentVoterIDs * fix verifier * fix mock * implement staking vote progressor * fix tests * move to module/packer * fix tests * fix validator test * fix validator test * use ParentVoterIndices * add QuorumCertificateWithSignerIDs * update blockSummary * move module * add test cases * remove comments * add check to EncodeSignerIndices * Apply suggestions from code review Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> * Apply suggestions from code review Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> * Apply suggestions from code review Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> * [Consensus] Use SignerIndices in CollectionGuarantee (3/4) (#2140) * implement IdentitiesByIndices * implement encoding and decoding * remove IdentitiesByIndices * use ParentVoterIndices * use chainID * add ChainID * fix test case * remove file * sort assignment to canonical order * fix guarantee * fix execution ingestion core * use signer indices * add identifierOrder * use guarantor.FindGuarantors * fix tests * fix tests * fix tests * disable tests * fix tests * fix tests, add validation * fix fvm test * fix access tests * fix tests * fix tests * fix select_filter_test tests * fix access tests * fix access integration * fix access integration tests * revert fvm_test changes * update fvm_test * fix exec tests * fix ingestion engine tests * fix test cases * remove todo * fix error handling * add comment * fix tests * update logging * refactor * fix tests * remove function * fix error wrapping * update comment * update tests * remove unverified signature * revert mutator tests change * update comment * address comments * add tests * add check in NewClusterList to ensure the assignments are sorted in canonical order * rename method * consensus ingestion core log signer indices * remove tests log * refactor canonical order check * replace order.ByNodeIDAsc with order.Canonical * [Consensus] Refactor for guarantee signer indices (4/4) (#2204) * wip * wip * starting to fix tests * adding tests * happy path test * Added toDo for fixing tests for unhappy paths * • fixed packer tests • consolidated logic for checking the padded bits * re-gen mocks * fix validPadding * fix findguarantors * refactor ingestion/core.go * move FindGuarantors to protocol * remove commented code * fix name * fix tests * small refactor * fix import * Apply suggestions from code review Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> * fix error type * fix type * fix error message * update comments * update comment * update tests * fix identity_test * fix tests * fix unittest * fix cluster tests * fix tests * fixtures ingestion engine tests * fix execution_test * fix consensus inclusion tests * fix bootstrap constraint check * fix cycle dep from NewClusterList (#2225) Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>

* update interface * remove IdentitiesByIndices * remove TODO * address review comments * [Consensus] SignerIndices Optimization (2/4) (#2101) * implement IdentitiesByIndices * convert signer index * implement encoding and decoding * add test cases * remove signer indices file * fix Packer * add comments * remove IdentitiesByIndices * update mock * fix test cases * add comments * remove fields * fix ParentVoterIDs * fix verifier * fix mock * implement staking vote progressor * fix tests * move to module/packer * fix tests * fix validator test * fix validator test * use ParentVoterIndices * add QuorumCertificateWithSignerIDs * update blockSummary * move module * add test cases * remove comments * add check to EncodeSignerIndices * Apply suggestions from code review Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> * Apply suggestions from code review Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> * Apply suggestions from code review Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> * [Consensus] Use SignerIndices in CollectionGuarantee (3/4) (#2140) * implement IdentitiesByIndices * implement encoding and decoding * remove IdentitiesByIndices * use ParentVoterIndices * use chainID * add ChainID * fix test case * remove file * sort assignment to canonical order * fix guarantee * fix execution ingestion core * use signer indices * add identifierOrder * use guarantor.FindGuarantors * fix tests * fix tests * fix tests * disable tests * fix tests * fix tests, add validation * fix fvm test * fix access tests * fix tests * fix tests * fix select_filter_test tests * fix access tests * fix access integration * fix access integration tests * revert fvm_test changes * update fvm_test * fix exec tests * fix ingestion engine tests * fix test cases * remove todo * fix error handling * add comment * fix tests * update logging * refactor * fix tests * remove function * fix error wrapping * update comment * update tests * remove unverified signature * revert mutator tests change * update comment * address comments * add tests * add check in NewClusterList to ensure the assignments are sorted in canonical order * rename method * consensus ingestion core log signer indices * remove tests log * refactor canonical order check * replace order.ByNodeIDAsc with order.Canonical * [Consensus] Refactor for guarantee signer indices (4/4) (#2204) * wip * wip * starting to fix tests * adding tests * happy path test * Added toDo for fixing tests for unhappy paths * • fixed packer tests • consolidated logic for checking the padded bits * re-gen mocks * fix validPadding * fix findguarantors * refactor ingestion/core.go * move FindGuarantors to protocol * remove commented code * fix name * fix tests * small refactor * fix import * Apply suggestions from code review Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> * fix error type * fix type * fix error message * update comments * update comment * update tests * fix identity_test * fix tests * fix unittest * fix cluster tests * fix tests * fixtures ingestion engine tests * fix execution_test * fix consensus inclusion tests * fix bootstrap constraint check * fix cycle dep from NewClusterList (#2225) Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>

* [Consensus] SignerIndices Optimization Interface changes (1/4) (#2047) * update interface * remove IdentitiesByIndices * remove TODO * address review comments * [Consensus] SignerIndices Optimization (2/4) (#2101) * implement IdentitiesByIndices * convert signer index * implement encoding and decoding * add test cases * remove signer indices file * fix Packer * add comments * remove IdentitiesByIndices * update mock * fix test cases * add comments * remove fields * fix ParentVoterIDs * fix verifier * fix mock * implement staking vote progressor * fix tests * move to module/packer * fix tests * fix validator test * fix validator test * use ParentVoterIndices * add QuorumCertificateWithSignerIDs * update blockSummary * move module * add test cases * remove comments * add check to EncodeSignerIndices * Apply suggestions from code review Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> * Apply suggestions from code review Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> * Apply suggestions from code review Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> * [Consensus] Use SignerIndices in CollectionGuarantee (3/4) (#2140) * implement IdentitiesByIndices * implement encoding and decoding * remove IdentitiesByIndices * use ParentVoterIndices * use chainID * add ChainID * fix test case * remove file * sort assignment to canonical order * fix guarantee * fix execution ingestion core * use signer indices * add identifierOrder * use guarantor.FindGuarantors * fix tests * fix tests * fix tests * disable tests * fix tests * fix tests, add validation * fix fvm test * fix access tests * fix tests * fix tests * fix select_filter_test tests * fix access tests * fix access integration * fix access integration tests * revert fvm_test changes * update fvm_test * fix exec tests * fix ingestion engine tests * fix test cases * remove todo * fix error handling * add comment * fix tests * update logging * refactor * fix tests * remove function * fix error wrapping * update comment * update tests * remove unverified signature * revert mutator tests change * update comment * address comments * add tests * add check in NewClusterList to ensure the assignments are sorted in canonical order * rename method * consensus ingestion core log signer indices * remove tests log * refactor canonical order check * replace order.ByNodeIDAsc with order.Canonical * [Consensus] Refactor for guarantee signer indices (4/4) (#2204) * wip * wip * starting to fix tests * adding tests * happy path test * Added toDo for fixing tests for unhappy paths * • fixed packer tests • consolidated logic for checking the padded bits * re-gen mocks * fix validPadding * fix findguarantors * refactor ingestion/core.go * move FindGuarantors to protocol * remove commented code * fix name * fix tests * small refactor * fix import * Apply suggestions from code review Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> * fix error type * fix type * fix error message * update comments * update comment * update tests * fix identity_test * fix tests * fix unittest * fix cluster tests * fix tests * fixtures ingestion engine tests * fix execution_test * fix consensus inclusion tests * fix bootstrap constraint check * fix cycle dep from NewClusterList (#2225) Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> * fix test cases * update flow dep * fix rpc convert * update go.mod * fix go.mod * fix testnet network * Remove unused travis yml * update mocks * upgrade flow * update mock * update go.sum * fix hotstuff integration tests * fix integration test * first commit * upgrade onflow/flow package to v0.3.0 (#2377) * add checksum * add checksum to signer indices * fix execution tests * fix execution tests * adding comments * fix ingestion tests * fix access tests * fix access tests * fix access tests * fix integration test * add validation * fix consensus integration tests * fix flakey tests * adding checks * fix flakey tests * fix check * fix order * fix sort * use crc32 to compute checksum * use canonical order * fix qc_test * Apply suggestions from code review Co-authored-by: Tarak Ben Youssef <50252200+tarakby@users.noreply.github.com> * update comments * added BackendSCciptsMetrics interface and update noop/collector * call the metrics * switched to counter * use BigEndian.PutUint32 * added buckets for script size * go fmt * added GetTransactionResultRTT metric * record GetTransactionResultRTT metric * record GetTransactionResultRTT metric * check unit tests * fix unit tests * improve signer indices logging * typo * moved start time into loop * go fmt * remove util * avoid race condition in validating blocks * add snapshot command * renamed methods and switched to histograms * add comment on usage * memory limit exception for service account * fixed typos (#2443) minor extension of error handling * fixed imports * address review comments * lint * fix comment * update tests for validPadding * refactor validPadding * chore(docker): fix parallel image builds * separated script size into two metrics * payload_size label * adding comments * chore(tests): fix load test Currently, test fails with the following error: ``` 5:21PM ERR account creation tx failed error="[Error Code: 1101] cadence runtime error Execution failed:\nerror: invalid public key: [248, 71, 184, 64, 62, 118, 6, 35, 180, 178, 53, 126, 38, 116, 202, 182, 91, 243, 157, 205, 89, 77, 199, 235, 192, 117, 38, 122, 5, 166, 65, 116, 192, 67, 8, 190, 19, 100, 174, 246, 253, 218, 181, 121, 11, 79, 74, 202, 139, 156, 247, 215, 109, 152, 90, 162, 48, 116, 13, 180, 101, 48, 1, 40, 106, 13, 47, 226, 2, 3, 130, 3, 232]\n --> 9d665e105234216749b51b3a301153c069b5c1047a1cf3f55e2ea38a5aa2c033:13:23\n |\n13 | let publicKey2 = PublicKey(\n | ^\n" ``` It looks like it tries to pass the encoded public key (along w/ algo, hash, and weight) to a function that expects a raw public key. Fix it by using the underlying PublicKey. * unquarantining 2 flaky tests that have been consistently passing in quarantine (#2455) * comments * fix function renaming * linter (#2457) * Add memory weight for atree encoded slabs * Add missing weights for cadence memory kinds * Update to latest cadence * Fix test * [BFT Testing] Corruptible Conduit Factory authenticating dictated messages. (#2441) * makes generating execution receipt exportable * (no branch): Auto stash before checking out "HEAD" * removes corrupted execution result * makes generating result approval exportable * adds result approval generation to ccf * fixes the tests * adds attestation fixture * adds message content as a parameter to message fixture * fixes bug with publish * wip * adds test for result approval * adds corrupted execution receipt test * adds todos to factory * refactors output types of wintermute orchestrator * fixes lint issue * removes unused variable * Update insecure/wintermute/helpers.go Co-authored-by: Misha <misha.rybalov@dapperlabs.com> * Update insecure/wintermute/helpers.go Co-authored-by: Misha <misha.rybalov@dapperlabs.com> * fixes lint * fixes receipt and approval bug with factory * adds pass through test for receipt * adds approval test * fixes lint * sorting imports * sorting imports Co-authored-by: Misha <misha.rybalov@dapperlabs.com> * go mod tidy * update Cadence * update atree Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Kay-Zee <kan@axiomzen.co> Co-authored-by: Daniel Holmes <danielholmes@dapper.local> Co-authored-by: Tarak Ben Youssef <50252200+tarakby@users.noreply.github.com> Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com> Co-authored-by: danielholmes839 <danielh839@gmail.com> Co-authored-by: Bastian Müller <bastian@axiomzen.co> Co-authored-by: Daniel Holmes <43529937+danielholmes839@users.noreply.github.com> Co-authored-by: Alexey Ivanov <SaveTheRbtz@GMail.com> Co-authored-by: Misha <misha.rybalov@dapperlabs.com> Co-authored-by: Supun Setunga <supun.setunga@gmail.com> Co-authored-by: Yahya Hassanzadeh <yahya@dapperlabs.com> Co-authored-by: Robert E. Davidson III <45945043+robert-e-davidson3@users.noreply.github.com>

zhangchiqing requested review from vishalchangrani, m4ksio, ramtinms and AlexHentschel as code owners March 11, 2022 00:29

zhangchiqing removed request for m4ksio, vishalchangrani, ramtinms and AlexHentschel March 11, 2022 00:38

zhangchiqing changed the title ~~[Consensus] Use SignerIndices in CollectionGuarantee~~ WIP [Consensus] Use SignerIndices in CollectionGuarantee Mar 11, 2022

zhangchiqing force-pushed the leo/guarantee-signer-indices branch from b6f6a8c to e1534f2 Compare March 11, 2022 22:20

zhangchiqing requested a review from janezpodhostnik as a code owner March 15, 2022 01:27

zhangchiqing removed the request for review from janezpodhostnik March 15, 2022 16:47

zhangchiqing changed the base branch from leo/signer-indices to feature/signer-indices March 15, 2022 17:02

zhangchiqing requested review from yhassanzadeh13 and jordanschalm as code owners March 15, 2022 17:02

zhangchiqing changed the base branch from feature/signer-indices to leo/signer-indices March 17, 2022 04:11

zhangchiqing commented Mar 17, 2022

View reviewed changes

zhangchiqing changed the title ~~WIP [Consensus] Use SignerIndices in CollectionGuarantee~~ [Consensus] Use SignerIndices in CollectionGuarantee Mar 17, 2022

zhangchiqing requested review from AlexHentschel, durkmurder and tarakby and removed request for yhassanzadeh13 March 17, 2022 04:50

durkmurder reviewed Mar 18, 2022

View reviewed changes

jordanschalm reviewed Mar 21, 2022

View reviewed changes

durkmurder reviewed Mar 22, 2022

View reviewed changes

zhangchiqing added 10 commits March 25, 2022 09:40

update logging

c4e63d4

refactor

6adfcdf

fix tests

b06fec6

remove function

9e45942

fix error wrapping

bf1ab10

update comment

1ee8ba8

update tests

7c88ade

remove unverified signature

553603a

revert mutator tests change

b885f9d

update comment

8dd8c35

zhangchiqing force-pushed the leo/guarantee-signer-indices branch from 7ef0931 to 8dd8c35 Compare March 25, 2022 16:41

jordanschalm reviewed Mar 28, 2022

View reviewed changes

zhangchiqing added 6 commits March 28, 2022 11:44

address comments

16bfbfc

add tests

69196e7

add check in NewClusterList to ensure the assignments are sorted in

fbd30bf

canonical order

rename method

34c8a03

consensus ingestion core log signer indices

ecabd59

remove tests log

765ea0e

zhangchiqing requested a review from jordanschalm March 28, 2022 19:25

jordanschalm approved these changes Mar 28, 2022

View reviewed changes

zhangchiqing and others added 3 commits March 28, 2022 15:48

refactor canonical order check

207fe59

replace order.ByNodeIDAsc with order.Canonical

770e549

zhangchiqing merged commit c94e689 into leo/signer-indices Mar 30, 2022

zhangchiqing deleted the leo/guarantee-signer-indices branch March 30, 2022 20:14

	indices, _ := packer.EncodeSignerIndices(list, 1)
	indices, _ := packer.EncodeSignerIndices(list, n)

	return engine.NewInvalidInputErrorf("cluster not found by chain ID %v, %v", guarantee.ChainID, err)
	return engine.NewInvalidInputErrorf("cluster not found by chain ID %v, %w", guarantee.ChainID, err)

		// determine whether signers reach minimally required stake threshold
		threshold := hotstuff.ComputeStakeThresholdForBuildingQC(clusterMembers.TotalStake()) // compute required stake threshold

	func QuorumCertificatesWithAssignments(assignment flow.AssignmentList) []*flow.QuorumCertificateWithSignerIDs {
	func QuorumCertificatesFromAssignments(assignment flow.AssignmentList) []*flow.QuorumCertificateWithSignerIDs {

	Signature: step.ParentVoterSigData, // TODO: to remove because it's not easily verifiable by consensus nodes
	Signature: nil, // TODO: to remove because it's not easily verifiable by consensus nodes

	return engine.NewInvalidInputErrorf("could not decode guarantor indices: %v", err)
	return engine.NewInvalidInputErrorf("could not decode guarantor indices: %w", err)

		assignment := identities[:]
		flow.IdentifierList(assignment).Sort(order.IdentifierCanonical)

	assignment := identities[:]
	flow.IdentifierList(assignment).Sort(order.IdentifierCanonical)
	assignment := flow.IdentifierList(identities[:]).Sort(order.IdentifierCanonical)

	if bytes.Compare(prev[:], participantID[:]) > 0 {
	if !IdentifierCanonical(prev, participantID) {

[Consensus] Use SignerIndices in CollectionGuarantee (3/4) #2140

[Consensus] Use SignerIndices in CollectionGuarantee (3/4) #2140

Conversation

zhangchiqing commented Mar 11, 2022 • edited Loading

github-actions bot commented Mar 15, 2022 • edited Loading

FVM Benchstat comparison

Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanschalm Mar 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangchiqing commented Mar 11, 2022 •

edited

Loading

github-actions bot commented Mar 15, 2022 •

edited

Loading

jordanschalm Mar 25, 2022 •

edited

Loading